Add entity tracing support - #296
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances distributed tracing for Durable Task entities by propagating incoming entity operation trace context to any outgoing signals/orchestration-start actions, and by extending TracingHelper with entity-specific OpenTelemetry span helpers plus accompanying unit tests.
Changes:
- Propagate
OperationRequest.trace_contextthrough entity-producedSendSignalActionandStartNewOrchestrationAction. - Add entity span utilities to
TracingHelper(span naming, processing spans, client/producer spans, schema-aligned attributes). - Add/extend unit tests validating entity trace propagation and entity span emission behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| client/src/main/java/com/microsoft/durabletask/TaskEntityExecutor.java | Captures per-operation trace context and attaches it to entity-produced signal/orchestration-start actions. |
| client/src/main/java/com/microsoft/durabletask/TracingHelper.java | Adds entity-specific tracing constants and helper methods for emitting entity-related spans. |
| client/src/test/java/com/microsoft/durabletask/TaskEntityExecutorTest.java | Adds tests asserting trace context propagation (and omission when absent) for entity-produced actions. |
| client/src/test/java/com/microsoft/durabletask/TracingHelperTest.java | Adds tests covering entity span naming and emission semantics (kinds, attributes, timestamps, error handling). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
client/src/main/java/com/microsoft/durabletask/TracingHelper.java:514
- These entity tracing helpers currently have no production callers (repository search finds only these definitions and
TracingHelperTest).DurableTaskGrpcWorkerstill invokesTaskEntityExecutor.executedirectly, while the orchestration and entity-client paths never call the new client/producer helpers, so enabling OpenTelemetry emits no entity spans at all. Please wire the processing, call, signal, and entity-start spans into those execution paths (and propagate the created span contexts), rather than leaving the implementation test-only.
static Span startEntityProcessingSpan(
String entityName,
String operation,
boolean signal,
String entityInstanceId,
@Nullable TraceContext parentContext) {
wangbill (YunchuWang)
left a comment
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Second-pass review focused on end-to-end entity tracing behavior and parity with the .NET implementation. I found two must-address completeness gaps, one timestamp issue that should be addressed, and one optional simplification. The context propagation logic itself otherwise looks safe and consistent with the .NET worker shim.
|
|
||
| // Parent context for any signals/orchestrations this operation produces, so the host can link them. | ||
| context.setCurrentOperationTraceContext( | ||
| opRequest.hasTraceContext() ? opRequest.getTraceContext() : null); |
There was a problem hiding this comment.
Must address or explicitly scope — the native orchestration-to-entity path cannot currently provide this context. In the ENTITYREQUESTV2 path, TaskOrchestrationExecutor creates EntityOperationCalledEvent / EntityOperationSignaledEvent, but neither protobuf message has a trace-context field. DurableTaskGrpcWorker can consequently copy only request ID, operation, and input into OperationRequest, making hasTraceContext() false for this path. The flattened request also loses whether the operation was a call or signal, so a processing span cannot select SERVER versus CONSUMER correctly.
Please define which component owns entity span creation and carry the required context/invocation kind through the protocol (with backend coordination), or explicitly limit this PR to propagation for backend-supplied V1 requests. An end-to-end test should cover both an orchestration call and signal.
There was a problem hiding this comment.
Rechecked at current head 27f6ea2a: this remains unresolved. The V2 conversion still builds both OperationRequest variants without a trace context, and TaskEntityExecutor now hardcodes signal=false when starting the processing span. Therefore the default native path still cannot link the incoming trace and can never produce a CONSUMER processing span for signals. The new test also explicitly expects zero CLIENT spans as "deferred pending protocol support". Please either coordinate the protobuf/backend fields needed for context + invocation kind, or explicitly re-scope and track this limitation before merging.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
wangbill (YunchuWang)
left a comment
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
Follow-up review of the remediation commits. The requestTime and Span.end(Instant) comments are correctly addressed, and processing/producer wiring has been added. The native V2 protocol limitation remains open (updated in the existing thread). I found three correctness gaps in the new wiring plus the still-deferred call/external-client coverage below. This is a comment-only review; it does not change the PR review state.
Also, GrpcDurableEntityClient.signalEntity() remains unchanged and still captures only the ambient context without emitting the new PRODUCER span, so external-client signals are not covered by the advertised entity tracing support. Please wire that path or explicitly document/re-scope it as deferred.
|
Follow-up at current head Correctly addressed and safe to keep resolved:
Still unresolved:
The current CI is green, but it does not exercise these failure and end-to-end hierarchy cases. I would keep the PR in changes-requested territory until these are fixed or the PR is explicitly re-scoped with tracked follow-ups. |
|
Thanks wangbill (@YunchuWang), here are the updates from my changes:
|
This pull request introduces enhancements to entity operation trace context propagation and adds comprehensive OpenTelemetry tracing support for Durable Task entities. The main changes ensure that when an entity operation includes a trace context, it is correctly propagated to any signals or orchestrations the entity produces. Additionally, the tracing helper now provides a full set of utilities for emitting and testing entity-related spans, aligning with the .NET SDK schema.
Entity Operation Trace Context Propagation:
TraceContextfrom an incoming entity operation is captured and propagated to outgoing signals and orchestrations, allowing for proper trace linkage in distributed tracing scenarios. (TaskEntityExecutor.java)TaskEntityExecutorTest.java)OpenTelemetry Tracing Support for Entities:
TracingHelper, following the .NET SDK schema. This includes methods for creating span names, starting/ending processing spans, and emitting client/producer spans for entity operations, signals, and orchestration starts.Pull request checklist
CHANGELOG.md